The latest Buf updates on your PR. Results from workflow CI and Release / buf-check (pull_request).

Claude finished @julienrbrt's task —— View job

Code Review Complete ✅

I've performed a comprehensive code review of this PR. Overall, this is a well-executed refactoring that successfully removes unnecessary complexity and improves the rollback functionality.

Summary of Changes

This PR accomplishes two main goals:

1. Removes --skip-p2p-stores flag: No longer needed after PR fix(block/syncing): save data to p2p stores #2736
2. Updates go-header dependency: Bumps to latest version from feat: Add DeleteRange method to Store celestiaorg/go-header#347

Detailed Review

✅ Positive Findings

1. Code Quality & Architecture

• ✅ Clean removal of obsolete skipP2PStores flag and associated logic
• ✅ Proper use of errors.Join() for error aggregation in rollback commands
• ✅ Improved error handling with better error messages
• ✅ Consistent implementation across both testapp and evm/single apps

2. Error Handling Improvements ⭐

• ✅ Excellent change in store.go:280: Now returns explicit error instead of silently accepting invalid rollback attempts (rolling back to same/higher height)
• ✅ Better error aggregation: Changed from early returns to collecting all errors with errors.Join(), allowing users to see all rollback failures at once
• ✅ Proper error wrapping with context using %w

3. State Management Improvements ⭐

• ✅ Significant improvement in store.go:325-327: Now deletes state at each height during rollback instead of trying to restore the target height's state
• ✅ This is more correct because it doesn't rely on the target state existing or being valid
• ✅ Simpler logic reduces potential failure points

4. Testing

• ✅ Test updates properly reflect the new behavior expectations
• ✅ Tests now correctly expect errors for invalid rollback attempts

5. Dependency Management

• ✅ Clean dependency version bump with proper TODO comment for tracking

🔍 Areas for Consideration

1. Error Handling in Rollback Commands ⚠️ Minor

In both apps/testapp/cmd/rollback.go:97-99 and apps/evm/single/cmd/rollback.go:91-93, the Start() method errors are now wrapped but the deferred Stop() calls don't check for errors:

if err := headerStore.Start(goCtx); err != nil {
    return err  // evm/single - good
}
defer headerStore.Stop(goCtx)  // Stop errors ignored

Recommendation: Consider capturing and logging Stop errors, especially if they might indicate resource leaks:

defer func() {
    if stopErr := headerStore.Stop(goCtx); stopErr != nil {
        fmt.Printf("Warning: failed to stop header store: %v\n", stopErr)
    }
}()

2. User Message Clarity ℹ️ Minor

The success message at apps/testapp/cmd/rollback.go:121-124 always suggests restarting with --clear-cache when syncNode is true, but this message only appears after potential errors. If there are errors in the errs variable, the user sees the success message followed by errors, which could be confusing.

Current flow:

fmt.Printf("Rolled back ev-node state to height %d\n", height)
if syncNode {
    fmt.Println("Restart the node with the `--clear-cache` flag")
}
return errs  // May contain errors

Recommendation: Consider only printing success message if errs == nil:

if errs == nil {
    fmt.Printf("Rolled back ev-node state to height %d\n", height)
    if syncNode {
        fmt.Println("Restart the node with the `--clear-cache` flag")
    }
}
return errs

3. Error Context in EVM Rollback ⚠️ Minor

In apps/evm/single/cmd/rollback.go:92-97, the Start() errors are wrapped with context, but the testapp version (lines 97-99) only returns the raw error. For consistency and better debugging:

evm/single (good):

if err := headerStore.Start(goCtx); err != nil {
    return fmt.Errorf("failed to start header store: %w", err)
}

testapp (could be improved):

if err := headerStore.Start(goCtx); err != nil {
    return err  // Less context
}

Recommendation: Add error wrapping in testapp for consistency.

🎯 Security & Robustness

✅ No security concerns identified

• ✅ No credential exposure risks
• ✅ Proper input validation (height checks)
• ✅ No panics or unsafe operations
• ✅ Context properly propagated for cancellation

📊 Testing Coverage

✅ Well-tested changes

• Tests updated to reflect new behavior
• Edge cases covered (rollback to same/higher height)
• All CI checks passing (Buf, Codecov)
• No coverage degradation

🎨 Code Style

✅ Follows Go best practices

• Idiomatic Go style
• Proper error handling patterns
• Clear variable naming
• Consistent formatting

📝 Documentation

✅ Comments and function docs are clear and accurate
✅ TODO comment properly tracks upstream dependency

Final Assessment

Overall: LGTM with minor suggestions ✅

This is a solid refactoring that:

• Removes unnecessary complexity
• Improves error handling and state management
• Maintains backward compatibility where appropriate
• Has good test coverage

The suggestions above are minor improvements and not blockers. The PR is ready to merge in its current state.

Files Reviewed:

• ✅ apps/testapp/cmd/rollback.go (+13/-18)
• ✅ apps/evm/single/cmd/rollback.go (+12/-17)
• ✅ pkg/store/store.go (+5/-21)
• ✅ pkg/store/store_test.go (+2/-2)
• ✅ apps/testapp/go.mod & go.sum (dependency update)
• ✅ apps/evm/single/go.mod & go.sum (dependency update)

Codecov Report

❌ Patch coverage is 25.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.16%. Comparing base (2c85e06) to head (d65867e).
⚠️ Report is 1 commits behind head on main.

@@            Coverage Diff             @@
##             main    #2744      +/-   ##
==========================================
- Coverage   62.17%   62.16%   -0.01%     
==========================================
  Files          79       79              
  Lines        8497     8485      -12     
==========================================
- Hits         5283     5275       -8     
- Misses       2721     2723       +2     
+ Partials      493      487       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.